[Draft] Evict the longer time stamp subscription #37025
Conversation
|
Review changes with |
| @@ -1128,6 +1128,15 @@ bool InteractionModelEngine::TrimFabricForSubscriptions(FabricIndex aFabricIndex | |||
| { | |||
| candidate = handler; | |||
| } | |||
| // The last report time of this handler is longer | |||
There was a problem hiding this comment.
Why is this the right logic? What is the actual behavior we are trying to implement?
If it's "evict the least newly created subscription", that's what the GetTransactionStartGeneration() thing above was trying to be a proxy for, but without adding new fields to ReadHandler, to avoid memory bloat. But if we are going to add new fields anyway, why wouldn't we just use those fields to track that information? But ideally we would not add fields.
If we really do want to evict handlers based on when they report, then this PR really needs to explain why that's a useful criterion, in the commit message or code comments (or maybe both).
There was a problem hiding this comment.
Maybe not the right logic.
GetTransactionStartGeneration returns the generation recorded during the creation of the ReadHandler. If the least newly created subscription has the same generation and resource usage as others, it will be evicted when a new subscription is added. This pattern would repeat, with each subsequent new subscription causing the eviction of the least recently created one. As a result, only the least recently created subscription and those created before the handler became full would remain active until the generation updates. Is this the intended behavior?"
There was a problem hiding this comment.
The current logic evicts the oldest subscription that satisfies the other criteria, right?
The new code being added would evict the subscription that reported least recently, assuming things are all the same generation. But why is when a subscription reported a relevant thing at all?
There was a problem hiding this comment.
Yes.
It has to report once receiving a new subscription.
Maybe we should try not to fill the ReadHandler instead of modifying the evict mechanism?
There was a problem hiding this comment.
I am not sure what you mean by "try not to fill the ReadHandler"...
There was a problem hiding this comment.
It means not to fill the ReadHandler pool in the InteractionModel.
DejinChen
changed the title
Current mechanism of
EnsureResourceForSubscriptionmay evict the Readhandler with more resources or less genaration (older). If same, the latest created one will be selected, in this case, only one subscription can be used for that the new subscription will evict the last one. So add a report timestamp and evict the longest timestamp subscription in this case.